Use a simpler :prefix-name: syntax for icon fonts by Xenonym · Pull Request #680 · MarkBind/markbind

Xenonym · 2019-02-08T04:46:39Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [X] Enhancement to an existing feature

Resolves #612.

What is the rationale for this request?
Currently, we use the {{ prefix_name }} syntax for inserting icon fonts from Font Awesome and Glyphicons. It would be ideal to support a simpler :prefix-name: syntax, similar to how we support :emoji:.

What changes did you make? (Give an overview)

I added markdown-it-regexp to match :prefix-name: and replace them with the appropriate icon font HTML.
I added aria-hidden="true" to the FA icons to be consistent with Glyphicons and for accessibility.
(FA's accessibility documentation, Bulletproof Accessible Icon Fonts)
I updated the default site-nav.md, the test_site's site-nav.md, and the user guide to use the new :prefix-name: syntax.
I removed the use of icon font variables from the default site, unit tests and test_site.
I created a corresponding PR Use a simpler :prefix-name: syntax for icon fonts in <panel> headings vue-strap#97 in the vue-strap repo to parse :prefix-name: in <panel> headers.

Provide some example code that this change will affect:

- FA university icon: :fas-university:
- Glyphicon icon: :glyphicon-home:

Testing instructions:

Add the above sample code to a MarkBind page and markbind serve. The icons should render:

acjh

Let's officially deprecate the variables syntax for icons.

yamgent

A search for {{glyphicon still returns files that are using the old syntax.

Chng-Zhi-Xuan · 2019-02-11T02:39:38Z

Seems like there are some icons that are not rendered.

In `reusingcontents.md`

Xenonym · 2019-02-11T08:57:29Z

Seems like there are some icons that are not rendered.

@Chng-Zhi-Xuan good catch! Turns out a preceding line break from the last <div> tag is needed, since we need to interpret the text as Markdown for markdown-it to parse the icon fonts.

Xenonym · 2019-02-11T09:30:02Z

A search for {{glyphicon still returns files that are using the old syntax.

@yamgent Yes, since these are still used to test variables. Since we are only deprecating the syntax for the next release, I think we still have to keep them to ensure that existing uses don't break until when we remove the syntax altogether. Also, there are some issues related to testing since we will not have static built-in variables after we remove the {{ prefix_name }} syntax. From the PR:

Some {{ prefix_name }} usages that were not changed because they are implicitly used to test variables:

In Site.js#FOOTER_DEFAULT:
'This is a dynamic height footer that supports variables {{glyphicon_tags}}'

In Site.test.js#test('Site resolves variables referencing other variables'):
'{{glyphicon_plus}}'

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

Chng-Zhi-Xuan · 2019-02-13T12:28:06Z

When we remove icon font variables, the only remaining built-in variable with HTML will be {{ MarkBind }}. Since {{ MarkBind }} changes with every new version, I am unsure if it is a good candidate for automated testing.

I think it is fine to swap over to the new syntax completely, We can just add unit tests in test_site to test any old syntax required.

acjh · 2019-02-13T13:58:16Z

@Chng-Zhi-Xuan By completely, do you mean including {{ MarkBind }}?

Note that this PR does not (should not) introduce a new syntax for variables, just "constant" icons.

Chng-Zhi-Xuan · 2019-02-13T15:15:45Z

@acjh I am referring to the Glyphicons using the old syntax. Since this PR is meant to improve icon syntax only.

Chng-Zhi-Xuan · 2019-02-14T07:39:04Z

@Xenonym, I looked through again and seems ok.

However, we should change our documentation and tests to follow the new icon syntax in this PR. "Implicit testing" of old syntax by intentionally leaving some icons with old syntax in templates or tests is not clean.

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

Once you're done, I can give the green light 👍

- aria-hidden=true is set for icon fonts for accessibility: https://www.filamentgroup.com/lab/bulletproof_icon_fonts.html#accessibility

- convert uses of {{ prefix_name }} to :prefix-name: - modify tests that rely on {{ prefix_name }} to use other variables - new example for variables that contain HTML tags to replace the old one which uses {{ prefix_name }}

Xenonym · 2019-02-15T06:59:12Z

Please do change the FOOTER_DEFAULT template in Site.js which uses icons, to the new syntax. I found other instances in Site.test.js and test_site using the old icon syntax as well.

@Chng-Zhi-Xuan made the necessary changes! Here's a summary:

Removed {{glyphicons_tag}} from FOOTER_DEFAULT. The use of {{MarkBind}} and {{timestamp}} already serve as a means to demonstrate variables.
Removed {{education_icon}} from test_site.
Modified test('Site resolves variables referencing other variables') to use a custom variable instead.
Used a  instead of  tag for the code example in reusingContents.md since the  tag is deprecated.

Chng-Zhi-Xuan

Thanks for the changes, looks good 👍

Good point on the  element being obsolete. Although I still find numerous instances of using it within our documentation / code base.

@Xenonym Do open up an issue to refactor  elements in a separate PR.

damithc · 2019-02-18T07:19:30Z

Gave it a try. Works as intended. Nice work @Xenonym and reviewers.

One small nit. This extra space can be misleading (not sure if it was introduced in this PR or not).

Chng-Zhi-Xuan · 2019-02-18T07:26:32Z

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

Snippet from icons.mbdf

   * _Solid_ (prefix: `fas-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`fas-`**`file-code`)
   * _Regular_ (prefix: `far-`) e.g., :fas-file-code: (actual name `file-code`, MarkBind name **`far-`**`file-code`)
   * _Brands_ (prefix: `fab-`): e.g., :fab-github-alt: (actual name `github-alt`, MarkBind name **`fab-`**`github-alt`)

damithc · 2019-02-18T07:38:40Z

That extra space was already present before the icon syntax change PR. The blank space is caused by the <code> tag having padding on the left and right side. There are 2 inline code blocks (fab- and file-code etc) so the result looks like an additional space in-between.

I guess it is a legacy problem. It's better to fix it though. We can sacrifice the bolding. On a related note, I hope we can add a proper way to highlight/bold parts of code some day.

The files were used as a template to build the iconsMap for using variable syntax to insert icons. Since MarkBind#680, we have shifted to another syntax for inserting icons. We made the decision to depreciate the old variable syntax. Thus it renders the current icon .csv files as obsolete. Let's remove these files as well when depreciating the variable syntax for icons.

Xenonym mentioned this pull request Feb 8, 2019

Use a simpler :prefix-name: syntax for icon fonts in <panel> headings MarkBind/vue-strap#97

Merged

damithc reviewed Feb 8, 2019

View reviewed changes

Comment thread docs/index.md Outdated

Chng-Zhi-Xuan self-requested a review February 8, 2019 06:04

acjh reviewed Feb 8, 2019

View reviewed changes

Comment thread src/lib/markbind/src/lib/markdown-it/index.js Outdated

yamgent requested changes Feb 10, 2019

View reviewed changes

Comment thread src/lib/markbind/src/lib/markdown-it/markdown-it-icons.js Outdated

Chng-Zhi-Xuan requested changes Feb 13, 2019

View reviewed changes

Comment thread docs/index.md

Xenonym added 3 commits February 15, 2019 14:40

Add markdown-it-regexp

497547e

Add parsing for new :prefix-name: icon font syntax

f83b685

- aria-hidden=true is set for icon fonts for accessibility: https://www.filamentgroup.com/lab/bulletproof_icon_fonts.html#accessibility

Update docs and tests to use new icon font syntax

2e610b5

- convert uses of {{ prefix_name }} to :prefix-name: - modify tests that rely on {{ prefix_name }} to use other variables - new example for variables that contain HTML tags to replace the old one which uses {{ prefix_name }}

Chng-Zhi-Xuan approved these changes Feb 15, 2019

View reviewed changes

Xenonym mentioned this pull request Feb 16, 2019

Replace in user docs with #696

Merged

yamgent added this to the v1.18.1 milestone Feb 17, 2019

yamgent merged commit dc53e76 into MarkBind:master Feb 18, 2019

Xenonym deleted the colon-iconfont-syntax branch February 18, 2019 02:35

This was referenced Feb 18, 2019

Remove stray space from FA icon syntax example #708

Merged

Remove variable syntax for icons #712

Closed

Upgrade Font Awesome from 5.1.0 to 5.7.2 #713

Merged

Chng-Zhi-Xuan mentioned this pull request May 21, 2019

Differentiate MarkBind syntax from Nunjucks? #859

Open

Chng-Zhi-Xuan mentioned this pull request May 21, 2019

Remove variable syntax for icons #864

Merged

Uh oh!

Conversation

Xenonym commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

acjh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yamgent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Chng-Zhi-Xuan commented Feb 11, 2019

Uh oh!

Xenonym commented Feb 11, 2019

Uh oh!

Xenonym commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Chng-Zhi-Xuan commented Feb 13, 2019

Uh oh!

acjh commented Feb 13, 2019

Uh oh!

Chng-Zhi-Xuan commented Feb 13, 2019

Uh oh!

Chng-Zhi-Xuan commented Feb 14, 2019

Uh oh!

Xenonym commented Feb 15, 2019

Uh oh!

Chng-Zhi-Xuan left a comment

Choose a reason for hiding this comment

Uh oh!

damithc commented Feb 18, 2019

Uh oh!

Chng-Zhi-Xuan commented Feb 18, 2019

Uh oh!

damithc commented Feb 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Xenonym commented Feb 8, 2019 •

edited

Loading

Xenonym commented Feb 11, 2019 •

edited

Loading